OEP-66 - update comment_client, add course_roles #556
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nt section of oep-66
robrap
approved these changes
Jan 24, 2024
There was a problem hiding this comment.
Note: Although I am approving, I think you could just leave off the PR links.
- The change history is more to alert people who were up-to-date on an OEP how to get back up to date.
- Git blame typically already contains the source of the change based on the change history PR: https://github.com/openedx/open-edx-proposals/blame/b1714976905aa949bf578539807da05d1669ea49/oeps/best-practices/oep-0066-bp-authorization.rst
feanil
approved these changes
Feb 2, 2024
|
|
||
| A course_roles_role can be assigned to a user in the LMS or CMS. | ||
| Some roles are granted in the LMS, some the CMS, and some both. | ||
| Which UI can be used to grant access will depend upon the values in the course_roles_roleservice database table. |
There was a problem hiding this comment.
Is there more you can say about the how it will depend on values in the course_roles_roleservice or is it possible to link to the relevant code or docs for how this works?
Comment on lines
435
to
439
| If a userrole is assigned to a course, it grants access based on the related permissions to that course. | ||
| If a userrole is assigned on an organization wide level, it grants access based on the related permissions to | ||
| all courses that belong to the organization. | ||
| If a userrole is assigned on an instance wide level, it grants access based on the related permissions to | ||
| all courses that belong to the instance. |
There was a problem hiding this comment.
Should this be a set of bullets as well?
|
This generally looks good to me as well, just had a couple of small questions. Not blocking if you want to leave things as they are. |
feanil
approved these changes
Feb 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
OEP-66 focuses on authorization best practices and includes descriptions of the currently in use systems. This PR adds additional information to the section on the roles used for discussions (django_comment_client_role) and adds an additional section on the proposed course_roles system.
Background
course_roles is intended to be used for course level access. The roles will be based upon permissions and access in the code will be gated by permission instead of role. They are intended to allow for greater flexibility in adding new roles. Information on the project can be found here and the tech spec is here.
To Do
last modified will need to be updated once any changes suggested on the PR are made